Skip to content

Develop#795

Open
Kacper-Leman wants to merge 10 commits into
mate-academy:masterfrom
Kacper-Leman:develop
Open

Develop#795
Kacper-Leman wants to merge 10 commits into
mate-academy:masterfrom
Kacper-Leman:develop

Conversation

@Kacper-Leman
Copy link
Copy Markdown

@Kacper-Leman Kacper-Leman commented May 22, 2023

Copy link
Copy Markdown

@DorotaLeniecDev DorotaLeniecDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix formatting and indentations, check this checklist https://github.com/mate-academy/layout_html-form/blob/master/checklist.md

When the code is more readable after fixes we will continue with review

Comment thread src/index.html Outdated
Comment on lines +17 to +19
<div>
Surname: <input type="text" name="Surname:">
</div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add proper labels
Fix formatting
Fix indentations

Suggested change
<div>
Surname: <input type="text" name="Surname:">
</div>
<div class="form__section_field">
<label for="surname">Surname: </label>
<input
type="text"
name="surname"
id="surname"
autocomplete="off"
>
</div>

Apply this pattern to all other fields
Remember to add spaces between sibling elements for better readability

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey, I make fixes please to check again :)

Copy link
Copy Markdown

@DorotaLeniecDev DorotaLeniecDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more changes needed

Comment thread src/index.html Outdated

<input
type="date"
name="date-birth"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name attibute should be camelCase

Suggested change
name="date-birth"
name="dateBirth"

Comment thread src/index.html Outdated
Comment on lines +77 to +79



Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one line space between siblings is needed

Comment thread src/index.html Outdated
Comment on lines +109 to +111



Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Comment thread src/index.html Outdated
Comment on lines +196 to +202
<textarea
name="comment"
id="comment"
cols="20"
rows="2"
autocomplete="off">
</textarea>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image Fix formatting

Comment thread src/index.html Outdated
Comment on lines +208 to +212
<select
name="recommend"
id="recommend"
autocomplete="off">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix formatting

Suggested change
<select
name="recommend"
id="recommend"
autocomplete="off">
<select
name="recommend"
id="recommend"
autocomplete="off"
>

Comment thread src/index.html Outdated
Comment on lines +113 to +114
<legend>An interesting fact about you!</legend>
<div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add empty line between siblings

Comment thread src/index.html Outdated
Comment on lines +81 to +82
<legend>Registration</legend>
<div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add empty line - double check if there are empty lines between siblings elements, do not add empty lines between parent and child

Comment thread src/index.html Outdated

<input
type="radio"
name="choice"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets make this name more descriptive so it is easier to use (in scenartio when form is being send)

Suggested change
name="choice"
name="lovesCats"

Comment thread src/index.html Outdated
<label for="yes">Yes</label>

<input type="radio"
name="choice"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Suggested change
name="choice"
name="lovesCats"

Comment thread src/index.html Outdated
<form action="https://mate-academy-form-lesson.herokuapp.com/create-application" class="form">
<fieldset>
<legend>Personal information</legend>
<div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be consistant and add class to all form fields wrappers. You can then specify the margins using pseudo selectors in css

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tips done I hope now its good.

Copy link
Copy Markdown

@DorotaLeniecDev DorotaLeniecDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more for perfection :)

Comment thread src/index.html Outdated

<input
type="color"
name="colour"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix typo

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be

Suggested change
name="colour"
name="color"

Comment thread src/index.html Outdated
autocomplete="off"
multiple
>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove space

Comment thread src/index.html Outdated
>
</div>

<div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion best way is to add form-field class to all divs and in css you can remove margin-bottom in :last-child selector. This is more consistant. It is a separation of duites, that styles will determin how the markup looks.

Comment thread src/index.html Outdated
Comment on lines +126 to +131
<input type="radio"
name="lovesCats"
id="no"
autocomplete="off"
value="No"
>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix formatting

Suggested change
<input type="radio"
name="lovesCats"
id="no"
autocomplete="off"
value="No"
>
<input
type="radio"
name="lovesCats"
id="no"
autocomplete="off"
value="No"
>

Comment thread src/index.html Outdated
id="recommend"
autocomplete="off"
>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove space

Copy link
Copy Markdown

@DorotaLeniecDev DorotaLeniecDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants